The first commit has the two overloads I think we definitely need:

• From(TreeEntry treeEntry) for the case above, where we just want to use an existing TreeEntry.

How about rather adding a new overload TreeDefinition.Add(string, TreeEntry)? Wouldn't that serve a similar purpose?

• From(ObjectId objectId) since we can't naturally get access to a Submodule instance during a rewrite.

Can't we use the existing TreeDefinition.AddGitLink(string, ObjectId) here?

How about rather adding a new overload TreeDefinition.Add(string, TreeEntry)?

👍

Can't we use the existing TreeDefinition.AddGitLink(string, ObjectId) here?

Not sure how I overlooked that... yes.

You could have just told me I was an idiot in the other PR. 😊

You could have just told me I was an idiot in the other PR. 😊

There was something bothering me about the initial proposal but I couldn't point at it earlier.

And, no. You're not an idiot.

🙉 🙈 🙊

Much smaller change for your consideration.

@dahlbyk Nice!

I was wondering if could take a fresh look at TreeDefinition.From(Tree). This calls an AddEntry method (that your code doesn't rely on).

• Either AddEntry is useful at this level, and maybe this new Add() overload should rely on it
• Or TreeDefinition.From(Tree) should directly call Add(string, TreeEntry) (As Add(string, TreeEntryDefinition) eventually seem to invoke AddEntry).

Last request: Could you please cover with some test this new overload. Imaginary bonus points if it demonstrates how simpler this makes the history rewriting process.

Could you please cover with some test this new overload.

Beyond the tests already included? I can add a filter-branch test as an example, too...

Either AddEntry is useful at this level, and maybe this new Add() overload should rely on it

This seems like the direction to go, with Add(string, TreeEntry) as a thin wrapper that provides validation. Though I'm not sure exactly what it is that the wrapped/unwrapped mechanism is doing, honestly.

Though I'm not sure exactly what it is that the wrapped/unwrapped mechanism is doing, honestly.

@dahlbyk A "wrapped" Tree is an already existing odb Tree. Once, you start altering it, by adding, removing or modifying any of its content, it "unwraps" (and also unwraps its parent Trees if any) and it's tracked as an entry in the unwrappedTrees dictionary.

Adding a new TreeEntryDefinition automatically replaces the potentially existing one. As such, AddEntry() takes care of removing any unwrapped Tree matching the key/name.

Considering this, I think it's safe to remove the call to AddEntry() from TreeDefintion.From(Tree). Could you please embed this as part of this PR?

diff --git a/LibGit2Sharp/TreeDefinition.cs b/LibGit2Sharp/TreeDefinition.cs
index 7426a28..63bbb0c 100644
--- a/LibGit2Sharp/TreeDefinition.cs
+++ b/LibGit2Sharp/TreeDefinition.cs
@@ -29,7 +29,7 @@ public static TreeDefinition From(Tree tree)

             foreach (TreeEntry treeEntry in tree)
             {
-                td.AddEntry(treeEntry.Name, TreeEntryDefinition.From(treeEntry));
+                td.Add(treeEntry.Name, treeEntry);
             }

             return td;

Considering this, I think it's safe to remove the call to AddEntry() from TreeDefintion.From(Tree). Could you please embed this as part of this PR?

Done

Rebased on latest. Also added a test that shows rewriting history to always use a specific version of a TreeEntry.

I love it!

Let's squash this! 🔨

Squashed first two...figure the last can stand alone.

Thanks 🍌 ‼️